feat: implement IGraphAlg generic dispatch and C wrappers#45
feat: implement IGraphAlg generic dispatch and C wrappers#45mahmudsudo wants to merge 5 commits intoJuliaGraphs:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45 +/- ##
=========================================
- Coverage 5.28% 4.86% -0.43%
=========================================
Files 8 8
Lines 4311 4459 +148
=========================================
- Hits 228 217 -11
- Misses 4083 4242 +159 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mahmudsudo
force-pushed
the
pr-graphs-api
branch
from
741034d to
6395d6b
Compare
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
Thanks for pushing on this @mahmudsudo ! We do not have many volunteer reviewers right now, so reminders on review are appreciated (and necessary, otherwise we just forget to do a review).
Could you help me a bit to understand how these things fit together, I see your PRs 43, 44, 45 and the Graphs.jl PR 506. Let's not worry about 506 for now. In 43 we discussed that the PR is rather large and dealing with multiple independent tasks so I asked to have it split so that it is easy to review it. Thank you for doing that, it does make things much easier for me to proceed with. However, the PR description above is empty, so I am not sure the overall goals you are pursuing -- could you enumerate the changes/reasons?
I also left a few minor comments in.
| return comps | ||
| end | ||
|
|
||
| #= |
There was a problem hiding this comment.
There is a lot of commented out code, what is the reason for it?
| @@ -1,34 +0,0 @@ | |||
| @testitem "Interface tests" begin | |||
There was a problem hiding this comment.
Why was this file deleted? I see you have created another related file. Could you keep the naming conventions the same and just update this file as appropriate.
There was a problem hiding this comment.
Could you check how this is done in Graphs.jl (in particular how it is addressed in the corresponding github workflows)? Could you stick to the same style? Having all the different projects in the JuliaGraphs org follow the same format would help when future org-wide changes need to be made.
See https://github.com/JuliaGraphs/Graphs.jl/blob/master/.github/workflows/ci-pre.yml#L30 and https://github.com/JuliaGraphs/Graphs.jl/blob/master/test/runtests.jl#L161
There was a problem hiding this comment.
(part of my goals here is to not have Pkg modify the test environment -- it causes a lot of cache/precompilation annoyances and slowdown in CI and confusing behavior when you run tests locally)
2 participants
No description provided.